[ESQL] Consolidate EsqlProject into Project#140238
Conversation
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Merges the EsqlProject class into Project, eliminating unnecessary class hierarchy after the QL merge. Key changes: - Move lenient expressionsResolved() behavior from EsqlProject to Project, allowing UnsupportedAttribute to pass through projections unchanged - Add ESQL_PROJECT_ENTRY to PlanWritables for backward compatibility when deserializing old "EsqlProject" plans from mixed-version clusters - Add checkUnsupportedAttributeRenaming() to Verifier to ensure renaming UnsupportedAttribute via Alias is still blocked (this check now runs unconditionally, not gated by resolved() status) - Update all instantiation sites to use Project instead of EsqlProject - Update comments referencing EsqlProject Closes elastic#111957
- Replace all EsqlProject references with Project in test assertions - Remove EsqlProjectSerializationTests (no longer needed) - Add regression tests in VerifierTests to validate that: - UnsupportedAttribute can pass through KEEP (Project) unchanged - Renaming UnsupportedAttribute fails even after passing through KEEP
61512a8 to
199dd85
Compare
...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/local/EsqlProject.java
Show resolved
Hide resolved
...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/local/EsqlProject.java
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java
Outdated
Show resolved
Hide resolved
idegtiarenko
left a comment
There was a problem hiding this comment.
Left couple of suggestions, overall looks good to me.
I am not changing this area often so it would be nice to have one extra review for this change.
…plan/logical/Project.java Co-authored-by: Ievgen Degtiarenko <ievgen.degtiarenko@gmail.com>
…plan/logical/Project.java Co-authored-by: Ievgen Degtiarenko <ievgen.degtiarenko@gmail.com>
4eabb62 to
d819361
Compare
We warn for unhandled annotations because of codegen
Test assertions will fail during deser if we create a `Project` with the name `EsqlProject` (old -> new transmission)
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java
Show resolved
Hide resolved
.../esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/MarkerAnnotationProcessor.java
Outdated
Show resolved
Hide resolved
.../esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/MarkerAnnotationProcessor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Should this be moved with the annotation in :libs:core?
Depending on if we can use UpdateForV10.class.getCanonicalName(), this processor may be difficult to find. Unless it fails on compile time anyway, which could work I guess.
| /** | ||
| * Backward compatibility entry + name for reading the consolidated `EsqlProject` plans from pre-9.4.0 nodes. | ||
| */ | ||
| private static final String LEGACY_PROJECT_NAME = "EsqlProject"; |
There was a problem hiding this comment.
Nit: If this is something that is targeted to ESQL team, I think LEGACY_LOGICAL_PLAN_NAME describes better the constant and intention.
alex-spies
left a comment
There was a problem hiding this comment.
LGTM. Thanks @MattAlp !
|
Ah, @MattAlp , because this affects serialization, you may want to run the CI with the |
This consolidates the
EsqlProjectclass intoProject, eliminating unnecessary class hierarchy originating from the QL merge. We've been meaning to do this for a while, #139248 did a lot of similar heavy lifting recently.The changes themselves:
expressionsResolved()behavior fromEsqlProjectintoProject, allowingUnsupportedAttributeto pass through projections unchangedESQL_PROJECT_ENTRYtoPlanWritablesfor backward compatibility when deserializing old"EsqlProject"plans from mixed-version clusterscheckUnsupportedAttributeRenaming()toVerifierto ensure renamingUnsupportedAttributeviaAliasis still blocked (this check now runs unconditionally, not gated byresolved()status)UnsupportedAttributepass-through and rename-blocking behaviorCloses #109195